refactor: migrate to more permissive/accurate String parameter types#161
refactor: migrate to more permissive/accurate String parameter types#161jschillem wants to merge 2 commits into
String parameter types#161Conversation
The API exclusively accepted `String` parameters before. This is forcing an unergonmic API for users by forcing them to allocate a `String` for an internal detail.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors various functions and API methods to use more idiomatic Rust types for string and path arguments, specifically replacing String with impl Into<String> and impl AsRef<Path>. This improves ergonomics by allowing callers to pass string literals or references without explicit conversions. However, several issues were identified in the src/ns.rs file, including a compilation error due to the use of non-existent methods on PathBuf and Path, as well as redundant path manipulation logic.
|
So you suggest me to make breaking changes and fancy trait argument just because unergonomic. No, this is premature optimization to me and solve no real problem. The allocation of String is always there because we need to generate netlink packet data, this change just move this allocation to somewhere else. Yes, we bump version always, but it does not mean we break API arbitrarily. |
I agree that the allocation is always there, however I'd argue that changing it to perform the allocation within the library code is useful to provide a better interface for users. Using the As for the breaking changes aspect, none of the trait bound arguments would be a breaking change. I completely understand being against the |
|
Please give me more time to check out how other famous crate handle this String argument stuff. For PathBuf, let's change it when real user complaint about it. |
2 participants
The API exclusively accepted
Stringparameters before. This is forcing an unergonomic API for users by forcing them to allocate aStringfor an internal detail.This PR changes all APIs that previously accept
Stringinto genericimpl Into<String>orimpl AsRef<Path>. These changes are backwards compatible asStringimplements both of these trait bounds.The only breaking change is converting the return type of
child_process_create_nsinns.rstoResult<PathBuf, Error>. This is more accurate to what should be returned as it is returning a file path. However, I am also wondering why this function is exposed as public while its wrapping functionchild_processis not.